Revision of tau quality cut configuration #30364

swozniewski · 2020-06-25T10:49:14Z

PR description:

Adds parameter maxDeltaZToLeadTrack to the tau quality cut descriptions. While technically usable by producers, this parameter was not used in the past and not represented in the fillDescriptions. In recent trigger studies, it is needed.

Furthermore, the part of fillDescriptions that refers to the qualityCuts and was present in several producers is moved to the RecoTauQualityCuts class and now imported and shared by the producers.

Some deprecated helper functions are removed from the RecoTauQualityCuts class.

This is only a technical change and no changes to physics results are expected.

PR validation:

compiles
code checks pass
format checks pass
unit tests pass
limited matrix tests pass

…lt disabled by setting its value to -1.0) - declare all configuration parameters related to RecoTauQualityCuts class in RecoTauQualityCuts.cc - removed obsolete global functions for applying quality cuts from RecoTauQualityCuts.cc

…o RecoTauQualityCuts class

…uested by Sebastian

cmsbuild · 2020-06-25T10:49:37Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-06-25T10:55:21Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30364/16419

This PR adds an extra 52KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File RecoTauTag/RecoTau/plugins/PFRecoTauChargedHadronProducer.cc modified in PR(s): [RFC] Test clang-tidy --checks modernize-make-unique #29934, [RFC] Test clang-tidy --checks modernize-loop-convert #29935, [RFC] Test clang-tidy --checks modernize-use-emplace #29937
- File RecoTauTag/RecoTau/plugins/PFRecoTauDiscriminationByIsolation.cc modified in PR(s): [RFC] Test clang-tidy --checks modernize-make-unique #29934, [RFC] Test clang-tidy --checks modernize-loop-convert #29935
- File RecoTauTag/RecoTau/plugins/PFRecoTauDiscriminationByIsolationContainer.cc modified in PR(s): [RFC] Test clang-tidy --checks modernize-make-unique #29934, [RFC] Test clang-tidy --checks modernize-loop-convert #29935, [RFC] Test clang-tidy --checks modernize-use-emplace #29937
- File RecoTauTag/RecoTau/plugins/PFRecoTauDiscriminationByNProngs.cc modified in PR(s): [RFC] Test clang-tidy --checks modernize-make-unique #29934
- File RecoTauTag/RecoTau/plugins/RecoTauPiZeroProducer.cc modified in PR(s): [RFC] Test clang-tidy --checks modernize-make-unique #29934, [RFC] Test clang-tidy --checks modernize-loop-convert #29935
- File RecoTauTag/RecoTau/plugins/RecoTauProducer.cc modified in PR(s): [RFC] Test clang-tidy --checks modernize-make-unique #29934, [RFC] Test clang-tidy --checks modernize-loop-convert #29935
- File RecoTauTag/RecoTau/src/PFTauPrimaryVertexProducerBase.cc modified in PR(s): [RFC] Test clang-tidy --checks performance-inefficient-vector-operation #29930, [RFC] Test clang-tidy --checks modernize-make-unique #29934, [RFC] Test clang-tidy --checks modernize-use-emplace #29937

cmsbuild · 2020-06-25T10:55:48Z

A new Pull Request was created by @swozniewski for master.

It involves the following packages:

RecoTauTag/RecoTau

@perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@riga this is something you requested to watch as well.
@silviodonato, @dpiparo you are the release manager for this.

cms-bot commands are listed here

slava77 · 2020-06-25T12:14:56Z

@cmsbuild please test

cmsbuild · 2020-06-25T12:15:23Z

The tests are being triggered in jenkins.

CMSSW_11_2_X_2020-06-25-1100/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/7342/console Started: 2020/06/25 14:22

cmsbuild · 2020-06-25T13:42:05Z

+1
Tested at: 4948d42
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-fa1200/7342/summary.html
CMSSW: CMSSW_11_2_X_2020-06-25-1100
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-06-25T13:42:08Z

Comparison job queued.

cmsbuild · 2020-06-25T15:38:14Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-fa1200/7342/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 36
DQMHistoTests: Total histograms compared: 2778915
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2778864
DQMHistoTests: Total skipped: 50
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 35 files compared)
Checked 152 log files, 16 edm output root files, 36 DQM output files

slava77

no actions are required for this PR.

However, the updates reveal some redundancy/duplication in the way how the configurations are made/used.
to which extent are the files created by fillDescriptions and modified in this PR actually used?
From a cursory check I see that some are used and some are not

pfTauPrimaryVertexProducer_cfi is not used
pfRecoTauChargedHadronProducer_cfi is used

Some cleanup/refactoring should be added to directly use the config files generated by the fillDescriptions.

slava77 · 2020-06-30T22:59:21Z

RecoTauTag/RecoTau/python/PFRecoTauQualityCuts_cfi.py

@@ -10,6 +10,7 @@
        maxTrackChi2                 = cms.double(100.),   # require track Chi2


this file better be populated from a reference file created by fillDescriptions.
The dual support of the content here and in fillDescriptions is not very practical.

slava77 · 2020-06-30T23:08:14Z

+1

for #30364 4948d42

code changes are in line with the PR description
- some further cleanup/refactoring of the tau configs should be planned to address Revision of tau quality cut configuration #30364 (review)
jenkins tests pass and comparisons with the baseline show no differences

cmsbuild · 2020-06-30T23:08:43Z

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo (and backports should be raised in the release meeting by the corresponding L2)

swozniewski · 2020-07-01T09:31:25Z

no actions are required for this PR.

However, the updates reveal some redundancy/duplication in the way how the configurations are made/used.
to which extent are the files created by fillDescriptions and modified in this PR actually used?
From a cursory check I see that some are used and some are not
* pfTauPrimaryVertexProducer_cfi is not used

* pfRecoTauChargedHadronProducer_cfi is used
Some cleanup/refactoring should be added to directly use the config files generated by the fillDescriptions.

I agree. We started a discussion internally how we want to include the autogenerated cfi files but did not want to delay this update. We have in mind (in addition to using only the auto-generated cfi-files of the producers) to make PFRecoTauQualityCuts create its own auto-generated cfi file (as you suggest above), where central modifications like for phase 2 can be applied and which is then shared between the producers like the current cfi for quality cuts.

slava77 · 2020-07-01T12:41:33Z

I agree. We started a discussion internally how we want to include the autogenerated cfi files but did not want to delay this update. We have in mind (in addition to using only the auto-generated cfi-files of the producers) to make PFRecoTauQualityCuts create its own auto-generated cfi file (as you suggest above), where central modifications like for phase 2 can be applied and which is then shared between the producers like the current cfi for quality cuts.

Some care may be needed here.
The current system would not let you to transparently use an auto-generated (helper) PFRecoTauQualityCuts_cfi from the fillDescriptions.
If a "helper" PSet is to be included in many different module definitions, then individual module fillDescriptions can include it by calling that helper::fillDescriptions method, which leaves a possibly auto-generated PFRecoTauQualityCuts_cfi useless by itself (the current situation, apparently, except that the module fillDescriptions-made files are not used directly).
The era/modifier updates will need to be added in every module fillDescriptions-generated file.
IIUC, self-contained helper support with era modifications in only one place will need that helper to become an ES or ED product producer (depending on event-data dependencies).
This way the full helper PSet will be moved out and can be modified with an era/modifier in one place.
@makortel @Dr15Jones what is your expectation/preference?

makortel · 2020-07-01T12:58:07Z

RecoTauTag/RecoTau/interface/RecoTauQualityCuts.h

@@ -91,6 +92,9 @@ namespace reco {
        return output;
      }

+      /// Declare all parameters read from python config file
+      static void fillDescriptions(edm::ParameterSetDescription& descriptions);


I'd suggest to rename this function to

Suggested change

static void fillDescriptions(edm::ParameterSetDescription& descriptions);

static void fillPSetDescription(edm::ParameterSetDescription& descriptions);

to follow a convention used elsewhere (including the helper plugins). The suggested name also signifies that the function fills a description for one PSet instead of arbitrary number of modules.

makortel · 2020-07-01T13:27:33Z

what is your expectation/preference?

I don't really have much to add. When a helper class (needing configuration) are used directly by many modules, and if it is expected that helper to configured consistently between the modules, any customization on the configuration of the helper needs to be repeated.

The repetition can be avoided by moving the helper algorithm to ED/ES product, in which case the configuration of the ED/ESProducer can be customized in a single place. The established practice is that if the helper needs event data, a new helper object should be produced on each event. If the helper does not need data from any of event, lumi, or run, it would be placed in EventSetup. In principle soon (after #30117 gets merged), a helper that depends only on configuration could be produced in beginProcessBlock transition.

silviodonato · 2020-07-03T19:13:53Z

+1

Christian Veelken added 6 commits June 25, 2020 11:44

fixed VPset for footprintCorrections

d5f13e2

switch to new method for declaring configuration parameters related t…

c6475b5

…o RecoTauQualityCuts class

improved code formatting by running 'scram build code-format', as req…

b347870

…uested by Sebastian

replaced pvPos_ by pv_->position() and removed pvPos_ data-member

a849f75

removed unnecessary include statement

4948d42

cmsbuild added this to the CMSSW_11_2_X milestone Jun 25, 2020

cmsbuild added code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Jun 25, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Jun 25, 2020

cmsbuild added tests-started and removed tests-pending labels Jun 25, 2020

cmsbuild added tests-approved and removed tests-started labels Jun 25, 2020

cmsbuild added comparison-available and removed comparison-pending labels Jun 25, 2020

slava77 reviewed Jun 30, 2020

View reviewed changes

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Jun 30, 2020

makortel reviewed Jul 1, 2020

View reviewed changes

cmsbuild mentioned this pull request Jul 3, 2020

[TEST ONLY][Do not merge] New clang tidy checks #30508

Closed

cmsbuild added orp-approved and removed orp-pending labels Jul 3, 2020

cmsbuild merged commit 0fdb1ad into cms-sw:master Jul 3, 2020

slava77 mentioned this pull request Feb 9, 2021

Preparations of deep tau ID for hlt phase2 (backport to CMSSW_11_1_X) #32831

Merged

mbluj deleted the CMSSW_11_2_X_tau-pog_QCutsCleaning branch October 10, 2023 10:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revision of tau quality cut configuration #30364

Revision of tau quality cut configuration #30364

swozniewski commented Jun 25, 2020

cmsbuild commented Jun 25, 2020

cmsbuild commented Jun 25, 2020

cmsbuild commented Jun 25, 2020

slava77 commented Jun 25, 2020

cmsbuild commented Jun 25, 2020 •

edited

Loading

cmsbuild commented Jun 25, 2020

cmsbuild commented Jun 25, 2020

cmsbuild commented Jun 25, 2020

slava77 left a comment

slava77 Jun 30, 2020

slava77 commented Jun 30, 2020

cmsbuild commented Jun 30, 2020

swozniewski commented Jul 1, 2020

slava77 commented Jul 1, 2020

makortel Jul 1, 2020

makortel commented Jul 1, 2020

silviodonato commented Jul 3, 2020

		@@ -10,6 +10,7 @@
		maxTrackChi2 = cms.double(100.), # require track Chi2

	static void fillDescriptions(edm::ParameterSetDescription& descriptions);
	static void fillPSetDescription(edm::ParameterSetDescription& descriptions);

Revision of tau quality cut configuration #30364

Revision of tau quality cut configuration #30364

Conversation

swozniewski commented Jun 25, 2020

PR description:

PR validation:

cmsbuild commented Jun 25, 2020

cmsbuild commented Jun 25, 2020

cmsbuild commented Jun 25, 2020

slava77 commented Jun 25, 2020

cmsbuild commented Jun 25, 2020 • edited Loading

cmsbuild commented Jun 25, 2020

cmsbuild commented Jun 25, 2020

cmsbuild commented Jun 25, 2020

slava77 left a comment

Choose a reason for hiding this comment

slava77 Jun 30, 2020

Choose a reason for hiding this comment

slava77 commented Jun 30, 2020

cmsbuild commented Jun 30, 2020

swozniewski commented Jul 1, 2020

slava77 commented Jul 1, 2020

makortel Jul 1, 2020

Choose a reason for hiding this comment

makortel commented Jul 1, 2020

silviodonato commented Jul 3, 2020

cmsbuild commented Jun 25, 2020 •

edited

Loading